feat(datasets): Refactored the ManagedTableDataset by Separating Out Common Table Logic #827

MinuraPunchihewa · 2024-09-08T09:15:01Z

Description

This PR refactors the ManagedTableDataset by separating our the common 'table' (managed and external) logic to a separate BaseTableDataset.

Development notes

The common table logic that is used to execute operations on Databricks Unity Catalog has been separated out here, with the primary purpose to be the implementation of an ExternalTableDataset in the near future.

Checklist

Opened this PR as a 'Draft Pull Request' if it is work-in-progress
Updated the documentation to reflect the code changes
Added a description of this change in the relevant RELEASE.md file
Added tests to cover my changes

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

…xist Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

MinuraPunchihewa · 2024-10-01T15:16:32Z

@noklam As requested, I have updated this PR to only contain the changes related to the refactor. From the CI runs, it is not entirely clear to me why the lint and unit tests are failing though. Is there anything else that I need to do here?

noklam

Generally looks fine for me. If you have to add new argument, make sure it's a non-breaking one, i.e. add it in the end instead of changing the existing order.

noklam · 2024-10-01T15:24:20Z

kedro-datasets/kedro_datasets/databricks/managed_table_dataset.py

+            catalog=catalog,
+            database=database,


Can the order be preserved? this would be a breaking change.

I just pushed a commit. Does this resolve your concern? Do the order of operations matter here since I am passing in keyword arguments?

The dataset class order doesn't matter because it is already a keyword only class.

The manage table class however is technically still a public class so change of order is a breaking change. It's not gonna affect too much since it's rare but I prefer only introduce breaking change when necessary.

@noklam Very clear. Thank you.

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

ankatiyar · 2024-10-02T08:37:02Z

Hey @MinuraPunchihewa, the refactoring looks good to me but the test coverage for kedro_datasets/databricks/_base_table_dataset.py is not complete which is causing the CI to fail. You might also need to run make plugin=kedro-datasets lint and push the linter changes.

MinuraPunchihewa · 2024-10-04T12:45:57Z

@ankatiyar @noklam I am working on the tests now. Can you please let me know how I can run them?
I tried make plugin=kedro-datasets test and pytest tests/databricks/test_managed_table_dataset.py, but I get the following error:

ERROR: usage: pytest [options] [file_or_dir] [file_or_dir] [...]
pytest: error: unrecognized arguments: --cov-report --cov-report term-missing --cov kedro_datasets --cov tests --no-cov-on-fail tests --cov-config pyproject.toml --numprocesses 4 --dist loadfile
  inifile: /home/minura/Documents/work/kedro/repos/kedro-plugins/kedro-datasets/pyproject.toml
  rootdir: /home/minura/Documents/work/kedro/repos/kedro-plugins/kedro-datasets

Is it possible to run only a particular test module with make?

noklam · 2024-10-04T12:58:54Z

Have you run make plugin=kedro-datasets install-test-requirements already? You are missing the testing dependencies (the error is complainign pytest-cov)

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

MinuraPunchihewa · 2024-10-04T14:40:03Z

@noklam Thank you; I was able to get it running after installing the dependencies.
@ankatiyar added some tests; what I basically did was take the existing tests for managed_table_dataset.py and moved it to _base_table_dataset.py. Then I added a simple tests for the describe() method of managed_table_dataset.py because that is the only place where it differs. Let me know what you think.
I've fixed the lint issues as well.

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

Signed-off-by: Minura Punchihewa <49385643+MinuraPunchihewa@users.noreply.github.com>

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

MinuraPunchihewa · 2024-10-07T06:32:35Z

Hey @noklam, @ankatiyar,
I needed to add a couple of other tests to get the coverage to 100%, but a couple of these tests require an external location to save the external tables in. I've added this in as a fixture which takes the location from an environment variable called DATABRICKS_EXTERNAL_LOCATION. Is it possible to set this up in Databricks environment configured for testing or is there a better way to go about this?

Signed-off-by: Ankita Katiyar <110245118+ankatiyar@users.noreply.github.com>

Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>

ankatiyar

Thanks @MinuraPunchihewa!

ankatiyar · 2024-10-10T16:51:53Z

I dont believe we have a test databricks setup but we can look at how to deal with the tests when you're ExternalTableDataset PR is ready. I'll try to get this refactor in for now.

MinuraPunchihewa · 2024-10-10T17:48:14Z

I dont believe we have a test databricks setup but we can look at how to deal with the tests when you're ExternalTableDataset PR is ready. I'll try to get this refactor in for now.

Thank you, @ankatiyar. Super stoked to have this merged.
Cool. May I know how the unit tests are running though? The unit tests that existed for ManagedTableDataset seemed to require a Spark environment to run from what I could gather.

MinuraPunchihewa added 29 commits September 8, 2024 14:45

added a base table implementation

fd0de72

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

added a base table dataset implementation

0392aba

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

renamed the module with the base classes

38c470f

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

refactored ManagedTable using BaseTable

d424829

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

refactored ManagedTableDataset using BaseTableDataset

7091986

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

removed primary_key attr from BaseTable

081d5d1

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

updated the format attrs of ManagedTable

e2cd580

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

implemented the _load() method of ManagedTableDataset

33814be

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

removed unnecessary imports

6f91dc8

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

reorganized the attrs of BaseTable

410cf4a

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

implemented create_table() in ManagedTableDataset

1bf0b8b

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

added the version attr to BaseTableDataset and updated load()

ee7e073

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

updated the base and managed datasets with all attrs

d87f4ed

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

updated the supported formats

a4feeff

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

added external table and external table dataset implementations

0db5639

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

added a val func to check for format when using upsert mode

e19eba4

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

imported the ExternalTableDataset into the main pkg

2c46978

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

improved the docstrings in the code

2777ea1

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

added format to the _describe()

6e1b17e

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

updated the save methods to incorporate partition columns

039f9e0

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

reverted the default write_mode back to None

fb87133

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

extended the _validate_write_mode() func to include formats

b7a5c33

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

updated the save() logic to work with single or multiple partition cols

577dd91

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

updated the docstrings for the datasets with missing attrs

6e729e3

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

introduced a location attr for creating ext tables

cbb8514

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

updated the save funcs to incorporate the locations attr

4a2dc6e

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

moved the func to check if table exists to BaseTable

1734f44

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

added a val func to check if location is provided if table does not e…

b03bee8

…xist Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

moved the val func for checking if write_mode supported to ExternalTable

dc3550e

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

MinuraPunchihewa force-pushed the feature/databricks_external_dataset branch from 880f599 to dc3550e Compare September 8, 2024 09:16

MinuraPunchihewa force-pushed the feature/databricks_external_dataset branch from 40cf9dc to fef688f Compare October 1, 2024 15:07

noklam reviewed Oct 1, 2024

View reviewed changes

MinuraPunchihewa force-pushed the feature/databricks_external_dataset branch 2 times, most recently from 7f81222 to fef688f Compare October 1, 2024 16:18

preserved order of args

8ee9ea8

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

noklam approved these changes Oct 1, 2024

View reviewed changes

MinuraPunchihewa added 3 commits October 4, 2024 20:04

updated the tests for base table dataset and managed table dataset

4cd7c0f

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

initialized the base table in the base table dataset

9982817

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

fixed lint issues

5a05a34

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

MinuraPunchihewa force-pushed the feature/databricks_external_dataset branch from 4941ab5 to 5a05a34 Compare October 4, 2024 14:34

Merge branch 'main' into feature/databricks_external_dataset

35bbe3b

MinuraPunchihewa and others added 5 commits October 4, 2024 20:17

fixed an incorrect type hint

c5d68ea

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

Merge branch 'main' into feature/databricks_external_dataset

e651008

Signed-off-by: Minura Punchihewa <49385643+MinuraPunchihewa@users.noreply.github.com>

removed redundant check on save()

b6dc5b9

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

added the missing unit tests

c5ea9d9

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

fixed the tests for saving external tables

0558d96

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

ankatiyar and others added 3 commits October 10, 2024 15:03

Merge branch 'main' into feature/databricks_external_dataset

145a2cf

Signed-off-by: Ankita Katiyar <110245118+ankatiyar@users.noreply.github.com>

lint

d598d79

Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>

Merge main

e34715a

Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>

ankatiyar approved these changes Oct 10, 2024

View reviewed changes

lrcouto merged commit d0d9f86 into kedro-org:main Oct 10, 2024
12 checks passed

MinuraPunchihewa mentioned this pull request Oct 11, 2024

feat(datasets): Added the Experimental ExternalTableDataset for Databricks #885

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(datasets): Refactored the ManagedTableDataset by Separating Out Common Table Logic #827

feat(datasets): Refactored the ManagedTableDataset by Separating Out Common Table Logic #827

MinuraPunchihewa commented Sep 8, 2024 •

edited

Loading

MinuraPunchihewa commented Oct 1, 2024

noklam left a comment

noklam Oct 1, 2024

MinuraPunchihewa Oct 1, 2024

noklam Oct 1, 2024

MinuraPunchihewa Oct 1, 2024

ankatiyar commented Oct 2, 2024 •

edited

Loading

MinuraPunchihewa commented Oct 4, 2024

noklam commented Oct 4, 2024

MinuraPunchihewa commented Oct 4, 2024

MinuraPunchihewa commented Oct 7, 2024

ankatiyar left a comment

ankatiyar commented Oct 10, 2024

MinuraPunchihewa commented Oct 10, 2024

feat(datasets): Refactored the ManagedTableDataset by Separating Out Common Table Logic #827

feat(datasets): Refactored the ManagedTableDataset by Separating Out Common Table Logic #827

Conversation

MinuraPunchihewa commented Sep 8, 2024 • edited Loading

Description

Development notes

Checklist

MinuraPunchihewa commented Oct 1, 2024

noklam left a comment

Choose a reason for hiding this comment

noklam Oct 1, 2024

Choose a reason for hiding this comment

MinuraPunchihewa Oct 1, 2024

Choose a reason for hiding this comment

noklam Oct 1, 2024

Choose a reason for hiding this comment

MinuraPunchihewa Oct 1, 2024

Choose a reason for hiding this comment

ankatiyar commented Oct 2, 2024 • edited Loading

MinuraPunchihewa commented Oct 4, 2024

noklam commented Oct 4, 2024

MinuraPunchihewa commented Oct 4, 2024

MinuraPunchihewa commented Oct 7, 2024

ankatiyar left a comment

Choose a reason for hiding this comment

ankatiyar commented Oct 10, 2024

MinuraPunchihewa commented Oct 10, 2024

MinuraPunchihewa commented Sep 8, 2024 •

edited

Loading

ankatiyar commented Oct 2, 2024 •

edited

Loading